Fix/distribution commission #217
Conversation
sh-cha
commented
Jun 27, 2024
sh-cha
commented
- ensure to sort when making decpools & pools
- add multi staking tests
WalkthroughWalkthroughThe updates primarily focus on enhancing the distribution module in several areas: adjusting bond denoms and reward weights for validators, optimizing decimal and pool sorting functionalities, and refining the commission withdrawal process. Additionally, there are multiple new test cases aimed at ensuring the correctness of these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Validator
participant Keeper
participant Rewards
User->>Validator: Stake tokens in various denoms
Validator->>Keeper: Set bond denoms and weights
Keeper->>Rewards: Allocate rewards based on new weights
Rewards->>Validator: Distribute rewards
Validator->>User: Rewards received
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
==========================================
+ Coverage 38.97% 40.22% +1.25%
==========================================
Files 251 258 +7
Lines 24165 24617 +452
==========================================
+ Hits 9418 9903 +485
+ Misses 13233 13183 -50
- Partials 1514 1531 +17
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- x/distribution/keeper/allocation_test.go (7 hunks)
- x/distribution/keeper/common_test.go (5 hunks)
- x/distribution/keeper/delegation_test.go (23 hunks)
- x/distribution/keeper/keeper.go (1 hunks)
- x/distribution/keeper/validator.go (1 hunks)
- x/distribution/types/dec_pool.go (5 hunks)
- x/distribution/types/dec_pool_test.go (1 hunks)
- x/distribution/types/pool.go (7 hunks)
- x/distribution/types/pool_test.go (1 hunks)
Files not summarized due to errors (1)
- x/distribution/keeper/delegation_test.go: Error: Message exceeds token limit
Files not reviewed due to errors (1)
- x/distribution/keeper/keeper.go (no review received)
Additional comments not posted (31)
x/distribution/types/pool_test.go (1)
1-268: Test cases for pool functionality are comprehensive and well-constructed.The tests cover various scenarios including equality, addition, and subtraction, which are crucial for ensuring the robustness of the
Poolsmethods. Ensure all new logic is covered, and consider adding more edge cases if necessary.
[APROVED]x/distribution/types/dec_pool_test.go (15)
35-82: Initialization logic in SetupSuite appears comprehensive.The method properly initializes various denominations and amounts, ensuring a wide range of test scenarios are covered.
84-102: Proper testing of DecPool equality.The
TestIsEqualPoolmethod correctly tests theIsEqualmethod ofDecPoolusing varied scenarios, ensuring robustness in equality checks.
104-117: Effective testing of pool emptiness.The
TestIsEmptyPoolmethod effectively checks the emptiness of pools using clear and concise test cases.
119-140: Thorough testing of the addition of DecPools.The
TestAddPoolmethod includes comprehensive test cases that cover normal operations and error conditions effectively.
142-163: Robust testing of the subtraction of DecPools.The
TestSubPoolmethod is well-structured and tests the subtraction functionality thoroughly, including error scenarios.
165-177: Effective validation of DecPools sorting upon creation.The
TestNewPoolsSortedmethod correctly ensures that new DecPools are sorted, which is crucial for performance and correctness in subsequent operations.
179-196: Comprehensive testing of adding multiple DecPools.The
TestAddPoolsmethod effectively tests the addition of multiple DecPools, ensuring correct and optimized handling of complex scenarios.
198-215: Robust testing of subtracting multiple DecPools.The
TestSubPoolsmethod thoroughly tests the subtraction of multiple DecPools, ensuring the functionality is reliable even in complex scenarios.
217-229: Effective detection of negative values in DecPools.The
TestIsAnyNegativePoolsmethod accurately identifies negative values in DecPools, which is essential for maintaining integrity in financial operations.
232-248: Accurate testing of coin retrieval from DecPools.The
TestCoinsOfPoolsmethod effectively tests the retrieval of coins based on denominations, ensuring accurate and efficient access to pool values.
250-263: Proper testing of emptiness for multiple DecPools.The
TestIsEmptyPoolsmethod correctly evaluates the emptiness of multiple DecPools, ensuring the functionality works as expected across different scenarios.
265-280: Thorough testing of equality for multiple DecPools.The
TestIsEqualPoolsmethod provides a comprehensive test of the equality functionality for multiple DecPools, ensuring accurate comparisons.
282-295: Effective testing of summing multiple DecPools.The
TestSumPoolsmethod successfully tests the summing of multiple DecPools, ensuring the operation is performed correctly and efficiently.
297-320: Robust testing of truncating decimal values in DecPools.The
TestTruncatePoolsmethod effectively tests the truncation of decimal values, ensuring precision is managed correctly in financial operations.
322-344: Effective testing of intersecting multiple DecPools.The
TestIntersectPoolsmethod accurately tests the intersection functionality for multiple DecPools, ensuring it works correctly for complex scenarios.x/distribution/keeper/common_test.go (2)
Line range hint
192-422: Comprehensive setup for testing multiple keepers.The
createTestInputfunction effectively sets up a complex test environment, initializing multiple keepers and ensuring their interactions are correctly configured.
610-613: Proper initialization of TestVotingPowerKeeper with default weights.The
NewTestVotingPowerKeepermethod correctly initializes theTestVotingPowerKeeperwith appropriate default weights, ensuring it is ready for use in tests.x/distribution/keeper/allocation_test.go (4)
156-197: Consistent addition of 'aaa' denom across multiple test configurations.Ensure the new denom 'aaa' is properly handled in all relevant test scenarios where
SetBondDenomsis used.Verification successful
Consistent addition of 'aaa' denom across multiple test configurations.
The new denom 'aaa' is properly handled in all relevant test scenarios where
SetBondDenomsis used.
x/distribution/keeper/allocation_test.gox/distribution/keeper/delegation_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new denom 'aaa' is properly handled across all test cases. rg --type go $'SetBondDenoms\(\.\.\.\)' x/distribution/keeper/allocation_test.goLength of output: 81
Script:
#!/bin/bash # Description: Verify that the new denom 'aaa' is properly handled across all test cases. rg --type go 'SetBondDenoms\('Length of output: 2066
81-81: Proper addition of 'aaa' denom to voting power weights.Verify that the new voting power weight for 'aaa' is consistently used in scenarios that depend on
SetVotingPowerWeights.Verification successful
Proper addition of 'aaa' denom to voting power weights.
The new voting power weight for 'aaa' is consistently used in both
delegation_test.goandallocation_test.go.
x/distribution/keeper/delegation_test.gox/distribution/keeper/allocation_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new voting power weight for 'aaa' is consistently used. rg --type go $'SetVotingPowerWeights\(\.\.\.\)' x/distribution/keeper/allocation_test.goLength of output: 89
Script:
#!/bin/bash # Locate the definition of SetVotingPowerWeights in the repository ast-grep --lang go --pattern 'func SetVotingPowerWeights($_, $_) { $$$ }' # Locate all usages of SetVotingPowerWeights across the repository rg 'SetVotingPowerWeights\('Length of output: 2897
Line range hint
170-213: Proper addition of 'aaa' denom to voting power weights in multiple test scenarios.Verify that the new voting power weight for 'aaa' is consistently used in scenarios that depend on
SetVotingPowerWeights.Verification successful
Proper addition of 'aaa' denom to voting power weights in multiple test scenarios.
The new voting power weight for 'aaa' is consistently used across various test files, confirming the changes have been properly integrated.
- Files Verified:
x/distribution/keeper/allocation_test.gox/distribution/keeper/delegation_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new voting power weight for 'aaa' is consistently used. rg --type go $'SetVotingPowerWeights\(\.\.\.\)' x/distribution/keeper/allocation_test.goLength of output: 89
Script:
#!/bin/bash # Description: Verify that the new voting power weight for 'aaa' is consistently used across the entire codebase. rg --type go 'SetVotingPowerWeights'Length of output: 2827
Line range hint
65-78: Addition of 'aaa' denom to bond denoms is consistent and well-implemented.Ensure that the new denom 'aaa' is properly handled in all relevant test scenarios where
SetBondDenomsis used.x/distribution/keeper/delegation_test.go (9)
21-36: Approved: Updates to bond denominations and reward weights inTestCalculateRewardsBasic.The addition of the "aaa" denomination and corresponding reward weights are well-integrated into the test setup. This change should help ensure that the new denomination is properly handled in reward calculations.
Also applies to: 38-44
145-159: Approved: Proper setup for testing rewards after slashing inTestCalculateRewardsAfterSlash.The changes to include the "aaa" denomination and adjust reward weights are crucial for testing the impact of slashing under new conditions. The test logic is sound and aligns well with the expected outcomes described in the PR.
Also applies to: 161-168
270-284: Approved: Configuration for multiple slashes scenario inTestCalculateRewardsAfterManySlashes.The inclusion of multiple slashes in this test function is a good stress test for the reward calculation logic under adverse conditions. The setup changes are consistent and necessary for the intended test coverage.
Also applies to: 286-293
428-442: Approved: Setup for multi-delegator reward calculation inTestCalculateRewardsMultiDelegator.The adjustments to include new denominations and corresponding reward weights are crucial for accurately testing the reward distribution among multiple delegators. This is a key aspect of ensuring the robustness of the distribution logic.
Also applies to: 444-451
570-584: Approved: Basic reward withdrawal setup inTestWithdrawDelegationRewardsBasic.The changes to include the "aaa" denomination in the setup are important for testing the basic functionality of reward withdrawals. This ensures that the system behaves as expected when new denominations are involved.
Also applies to: 586-593
675-689: Approved: Zero rewards withdrawal test setup inTestWithdrawDelegationZeroRewards.Including tests for zero rewards is an important part of ensuring robustness in the rewards system, particularly with the introduction of new denominations. The setup changes are appropriate and necessary for this test scenario.
Also applies to: 691-698
745-759: Approved: Setup for testing multiple slashes in the same block inTestCalculateRewardsAfterManySlashesInSameBlock.Testing multiple slashes in the same block is a critical scenario for understanding the robustness of the reward calculation system. The setup changes are well thought out and align with the objectives of the test.
Also applies to: 761-768
892-906: Approved: Complex test setup for multiple delegators and slashes inTestCalculateRewardsMultiDelegatorMultiSlash.The setup for this function is comprehensive, covering multiple delegators and multiple slashes, which is essential for testing the system's handling of complex scenarios. The changes are well-executed and align with the test's objectives.
Also applies to: 908-915
1056-1070: Approved: Multi-delegator, multi-withdrawal test setup inTestCalculateRewardsMultiDelegatorMultWithdraw.The setup for testing multiple withdrawals by multiple delegators is crucial for ensuring the distribution system's functionality under varied operational scenarios. The changes are appropriate and align with the testing goals.
Also applies to: 1072-1079
| } else { | ||
| current = append(current, customtypes.NewDecPool(token.Denom, rewardCoins.QuoDecTruncate(math.LegacyNewDecFromInt(token.Amount)))) | ||
| current = current.Add(customtypes.NewDecPool(token.Denom, rewardCoins.QuoDecTruncate(math.LegacyNewDecFromInt(token.Amount)))) |
There was a problem hiding this comment.
Tip
Codebase Verification
Inconsistent use of division methods in financial calculations.
The QuoDecTruncate method is only used in one instance, while other instances use QuoDec and Div without truncation. This inconsistency could lead to different rounding behaviors and potential issues in financial calculations. Review and standardize the use of division methods across the codebase.
x/mstaking/keeper/delegation_test.gox/move/ante/gas_prices_test.gox/move/ante/gas_prices.go
Analysis chain
Approved change in reward calculation method.
The use of QuoDecTruncate for calculating the reward distribution is a safer choice to prevent rounding errors. Ensure this change aligns with other financial calculations across the system.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that QuoDecTruncate is used consistently across similar financial calculations.
# Test: Search for similar usages of division in financial calculations.
rg --type go $'QuoDecTruncate'
Length of output: 194
Script:
#!/bin/bash
# Description: Search for other methods of division in financial calculations to identify inconsistencies.
# Test: Search for other division methods in financial calculations.
rg --type go 'QuoDec\(|QuoDecTruncate\(|Div\('
Length of output: 1727
| // NewPools creates a new Pools instance | ||
| func NewPools(pools ...Pool) Pools { | ||
| return removeZeroPools(pools).Sort() | ||
| } |
There was a problem hiding this comment.
Enhancements to pool handling and safety checks approved.
The changes ensure that pools are always sorted and zero pools are removed before operations, which enhances safety and consistency. Consider adding comments explaining why sorting is necessary before operations like addition.
+ // Ensure pools are sorted to maintain consistency and prevent errors in set operations.
+ // Removing zero pools to clean up the data before processing.Also applies to: 34-65, 205-222
| @@ -19,7 +19,7 @@ | |||
| decPools = append(decPools, NewDecPool(p.Denom, sdk.NewDecCoinsFromCoins(p.Coins...))) | |||
| } | |||
|
|
|||
| return decPools | |||
| return decPools.Sort() | |||
There was a problem hiding this comment.
Enhancements to decimal pool handling and safety checks approved.
The changes ensure that decimal pools are always sorted and zero pools are removed before operations, which enhances safety and consistency. Consider adding comments explaining why sorting is necessary before operations like addition, similar to pool.go.
+ // Ensure decimal pools are sorted to maintain consistency and prevent errors in set operations.
+ // Removing zero decimal pools to clean up the data before processing.Also applies to: 41-283
## Description Closes: MILK-104 This PR cherry picks the changes made by the Initia team to `Pools` and `DecPools` in this [PR](initia-labs/initia#217). --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/milkyway-labs/milkyway/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://docs.cosmos.network/v0.44/building-modules/intro.html) - [ ] included the necessary unit and integration [tests](https://github.com/milkyway-labs/milkyway/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [x] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)
